Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor module loading v2: move writing of module_config.bin to bootstrap #425

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dale-wahl
Copy link
Member

I was looking over PR #396, updated it from the master, but ran into some issues. The "do not lazy load" commit (71ba788) causes failures since self.modules is often None. When I dove a bit deeper I became a little unsatisfied with how often ModuleCollector was being initialized and doing all that work. Before we would import the same instantiated object from backends instead of passing it around. I could do some more work here, but I think this PR's method may accomplish the same thing in a more simple fashion.

This PR only writes the module_config.bin in bootstrap and then updating the shared config object.

Both this solution and the other PR mean that the frontend will only have the latest data from module_config.bin if it is started after the backend. I do not believe this is a large problem as module_config.bin is only modified if code is updated (necessitating a restart of both anyway), but worth noting for 4CAT instances not managed by Docker as order becomes important.

As you are well aware, both PRs have an oddity in that all module classes are imported with a version of config that does not have the module configurations. The only place I noticed an effect was in ModuleCollector.expand_datasources() which calls get_options(). That method does often use config, but in expand_datasources we only care about any options returning so there seems to be no issue. I note it in case something happens down the road related to this.

@dale-wahl dale-wahl requested a review from stijn-uva April 16, 2024 10:47
stijn-uva added a commit that referenced this pull request Sep 19, 2024
In spirit, at least
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant